-
Notifications
You must be signed in to change notification settings - Fork 65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add documentation on the expectations of vendor images. #535
Conversation
Pull Request Test Coverage Report for Build 9128632529Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
docs/vendor.md
Outdated
Vendors are responsible for support of their images. This includes the node | ||
implementation in | ||
[kne/topo/node](https://github.com/openconfig/kne/tree/main/topo/node) as well | ||
as the vendor specific examples in | ||
[kne/examples](https://github.com/openconfig/kne/tree/main/examples). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are community contributions to vendor node
implementations welcome? Do you expect or require vendors to provide review on community contributions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think community contributions should be welcome though the vendor should be the one to approve changes to code they are responsible for. I will updated the document.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree in principle, but I would caution about being too dogmatic about this. For example, I have a vendor who is not inclined to provide support for one containerized platform of theirs in KNE; I have local (not ready to publish, but do intend to) modifications to provide that support and I would be concerned if I seemed there was limited hope of this being accepted upstream given the vendors' apparent reticence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the question will come down to who has taken the responsibility to maintain that code. If the ACME Network Switch company has provided a node implementation (implying they are the maintainer of that code) for most of their switches and you want to add in code to support one of the missing switches you have two paths. The best option is to provide a PR that updates their node implementation and get their approval (as they maintain that node implementation). If that path is not possible there can always be a new node implementation offered, though the benefits of this would need to out weigh the costs and technical debt. If a the maintainer of a node implementation is non-response then you could take over maintenance of that code.
…ository are welcome and that the vendors should review and approve those PRs.
docs/vendor.md
Outdated
to be used with KNE. | ||
|
||
This document describes the image requirements and expectations in order to be | ||
considered *KNE Qualified*. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does KNE qualified mean in this case? Will there be an indicator anywhere that the image is KNE qualified, as in, is this the requirements for us to merge a new node impl? Or do mark some of the node impls as "qualified"?
docs/vendor.md
Outdated
## Testing | ||
|
||
Vendor images must be tested prior to publication. A standard set of tests is | ||
found at <INSERT LOCATION HERE>. It is expected that the image undergoes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which tests are intended here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure these tests yet exist, though at a minimum KNE when the image is used bringing up KNE should not hang or crash because of the image, nor prevent other images from being used.
Add documentation on the expectations of vendor images.